FEAT: Implementing is_integer and as_integer_ratio for QuadPrecision
#221
Conversation
| } | ||
| } | ||
|
|
||
| // this is thread-unsafe |
There was a problem hiding this comment.
@ngoldbaum precisely, Sleef_snprintf here is thread-unsafe, I believe just GIL won't help here as this is C routine and GIL only protects the Python objects.
Not 100% sure so need your opinion that whether GIL would be enough or we can lock this region with pthread_mutex
There was a problem hiding this comment.
I guess that means that it's not safe to concurrently call Sleef_snprintf simultaneously in two threads (e.g. it's not re-entrant)?
In that case, yeah, I think you need a global lock.
I'd avoid using pthreads directly because then you'd need to do something else on Windows. Instead, I'd use PyMutex on Python 3.13 and newer and PyThread_type_lock on 3.12 and older. See e.g. the use of lapack_lite_lock in numpy/linalg/umath_linalg.cpp in NumPy, which solves a similar problem with the non-reentrant lapack_lite library.
You could also switch to C++ and use a C++ standard library but then you need to be careful you don't deadlock with the GIL by making sure you release it before doing any possibly blocking calls. The built-in lock types have deadlock protection against the GIL so you don't need to go through that trouble.
There was a problem hiding this comment.
Also it'd be nice to add a multithreaded test for this as well. You can look at test_multithreading.py in NumPy for some patterns to use for multithreaded tests.
There was a problem hiding this comment.
Pushed a multithreaded test for now only for testing this. Will push the lock after the tests get done
|
It seems not to be crashing somehow @ngoldbaum can you check the |
|
Did you try running under TSan? You'll need to build python, numpy, SLEEF, and numpy-quaddtype all with the same compiler/sanitizer stack. See https://py-free-threading.github.io/thread_sanitizer/ |
|
Do we need to add this in CI? |
|
I don't think so, a one-off local test every now and then is fine. It may make sense to add TSan testing along with supporting the free-threaded build and more extensive multithreaded testing, but not until a lot of work has happened towards that. |
|
cool, so I can push the lock adding commit |
Did this on macos and running Pytest didn't give any TSan warnings for |
|
I'll push the code with the commented options for building with TSan so that in near future, anyone wants just uncomment and test |
|
Yes; stubtest does what it's supposed to do now 🎉 The errors can easily be fixed by copying these method stubs over from numpy: |
|
@ngoldbaum is this ready to merge? |
|
@jorenham can you please take a look at this error in validating static types? |
The added methods need to be decorated with |
| @@ -1,5 +1,5 @@ | |||
| from typing import Any, Literal, TypeAlias, final, overload | |||
|
|
|||
| import builtins | |||
There was a problem hiding this comment.
Why the explicit import? We use bool without the builtins.bool elsewhere in the stubs
There was a problem hiding this comment.
Copied from the NumPy's stubs, I think both are just same.
There was a problem hiding this comment.
in numpy it's needed because bool is shadowed by np.bool. But that shouldn't be problem here, so no need for the builtins._
|
I try to avoid looking at code on weekends - I'll look at this next week. |
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
Sorry for taking a little while to look at this. I wanted to have time to run TSan testing locally.
For what it's worth, on the free-threaded build, I see a data race inside Sleef_iunordq1 on a global value in the new multithreaded test you added. Possibly this is shibatch/sleef#560. We should probably report it upstream as a bug.
|
That's great capture, I was using the SLEEF from the subproject itself, thanks a lot @ngoldbaum Regarding SLEEF (I didn't know pytorch uses it) but in QBLAS I am also using vectorized functions. I might need to evaluate this part extensively as from the related issues and this one itself. We are on SLEEF v3.8 (LTS is 3.9, but focussed on DFT not Quad) in short optimal fixes might can take time, but the current ones are resolvable |
|
@ngoldbaum So I think the workflow would be: both the T14 and T74 threads triggered I will report this to SLEEF, and I think till then in ours we have 2 options
PyMODINIT_FUNC
PyInit_quaddtype(void)
{
PyObject *m;
// ... other initialization ...
// SLEEF init
Sleef_quad dummy = sleef_q(1LL, 0ULL, 0);
Sleef_quad dummy2 = sleef_q(2LL, 0ULL, 0);
int dummy_int;
// Warmup all SLEEF functions you use
Sleef_iunordq1(dummy, dummy2);
Sleef_icmpgeq1(dummy, dummy2);
Sleef_fabsq1(dummy);
Sleef_frexpq1(dummy, &dummy_int);
Sleef_cast_from_doubleq1(1.0);
return m;
}This might not gurantee the other races if function implementation itself had issues |
|
Interesting, I am somehow still now able to see the race condition on x86-64 machine |
Do you mean you're not able to trigger it? Certain kinds of data races are only possible on ARM machines. x86_64 CPUs don't support weak memory ordering. IMO you shouldn't do anything special inside quaddtype to handle this problem. I think the race might be benign in that both threads will end up writing the same result to the function pointer. Instead, I'd report it upstream, ideally with a reproducer written using e.g. So I'd say to go ahead and merge this as-is without implementing your fix to initialize the pointers in module initialization. |
|
Yeah I just setup on my mac and ran, now got a lot of races and all related to the dispatching of Sleef functions like |
Sure, I'll resolve the other comments and README section for TSan build and then merge it in |
Right, I also think that since my x86-64 machine supports |
|
Cool @ngoldbaum if you take a look at the README then this is good to merge |
4 participants
closes #216
I took the reference for implementing
as_integer_ratiofrom CPython's implementation